GH-50915: [FORMAT] Allow TIMESTAMP logical type to annotate FIXED_LEN_BYTE_ARRAY(12) - #50916
GH-50915: [FORMAT] Allow TIMESTAMP logical type to annotate FIXED_LEN_BYTE_ARRAY(12)#50916divjotarora wants to merge 3 commits into
Conversation
|
|
github-actions
Bot
added
awaiting review
emkornfield
left a comment
emkornfield
left a comment
There was a problem hiding this comment.
Some questions and comments I think the biggest one is scope and whether we should have an option to convert this value to a proper arrow type. Wemight also want to make it configurable the target of the arrow type
github-actions
Bot
added
awaiting committer review
| return Status::Invalid( | ||
| "FLBA(12) TIMESTAMP value does not fit in a 64-bit Arrow timestamp"); | ||
| } | ||
| const bool negative = (bytes[11] & 0x80) != 0; |
There was a problem hiding this comment.
why not re-use low_signed and do a copmarison against it?
| const uint32_t high = bit_util::FromLittleEndian(SafeLoadAs<uint32_t>(bytes + 8)); | ||
| const int64_t low_signed = static_cast<int64_t>(low); | ||
| // Fits in int64 iff the high part is a pure sign-extension of the low part. | ||
| if (static_cast<int32_t>(high) != (low_signed < 0 ? -1 : 0)) { |
There was a problem hiding this comment.
nit: separate (low_signed < 0 ? -1 : 0) to its own variable 'sign_extension'
|
|
||
| return ::arrow::fixed_size_binary(physical_length); | ||
| case LogicalType::Type::TIMESTAMP: | ||
| // If configured, convert to a potentially lossy Arrow timestamp. Otherwise, return |
There was a problem hiding this comment.
nit: maybe simplify the comment to just note this path is loss, otherwise I think the logic is pretty much self documenting?
| should_load_statistics_(false), | ||
| smallest_decimal_enabled_(false) {} | ||
| smallest_decimal_enabled_(false), | ||
| convert_flba_timestamps_(false), |
There was a problem hiding this comment.
I think it is probably more useful to default this to true, most timestamps will likely be in a range that is handle-able, and if the intent is for it to be timestamp then I think this makes a better end-user experience. CC @wgtmac @pitrou do you have a preference here?
Keeping the clamp_on_overflow to false makes sense to me because otherwise it is potential corruption/data-loss.
| bool clamp_on_overflow) { | ||
| static const auto binary_type = ::arrow::fixed_size_binary(12); | ||
| std::shared_ptr<ChunkedArray> chunked_array; | ||
| RETURN_NOT_OK( |
There was a problem hiding this comment.
Do we really need to transfer here, can't we read with the underlying reader?
| const bool negative = (bytes[11] & 0x80) != 0; | ||
| builder.UnsafeAppend(negative ? INT64_MIN : INT64_MAX); | ||
| } else { | ||
| builder.UnsafeAppend(low_signed); |
There was a problem hiding this comment.
I think we probably need to multiply/divide based on the logical arrow type and logical parquet type? Maybe this is an argument to limit the FLBA type to nanosecond and finer granularities when they exist?
emkornfield
left a comment
emkornfield
left a comment
There was a problem hiding this comment.
I think the conversion code always assumes a 1:1 mapping between arrow timestamp granularity and parquet granularity. I think in the common path when schema is inferred this is workable, but IIRC users can also supply there own schema (we should add test coverage for this path).
| /// \brief Set whether to infer Arrow timestamps from Parquet FLBA types. | ||
| /// | ||
| /// When enabled, Parquet FLBA(12) TIMESTAMP columns are read as Arrow timestamps. | ||
| /// VAlues that do not fit in 64 bit timestamps are handled per |
There was a problem hiding this comment.
| /// VAlues that do not fit in 64 bit timestamps are handled per | |
| /// Values that do not fit in 64 bit timestamps are handled per |
| should_load_statistics_(false), | ||
| smallest_decimal_enabled_(false) {} | ||
| smallest_decimal_enabled_(false), | ||
| convert_flba_timestamps_(false), |
3 participants
Rationale for this change
See apache/parquet-format#600 for rationale.
What changes are included in this PR?
This PR adds support for using
TimestampTypeto annotateFIXED_LEN_BYTE_ARRAY(12)values.Are these changes tested?
Yes, via unit tests and an e2e test that reads the file added in parquet-testing (apache/parquet-testing#123).
Are there any user-facing changes?
No